Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix matchResponseMapWithRequests to handle prompt response. #505

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Fix matchResponseMapWithRequests to handle prompt response. #505

wants to merge 5 commits into from
+29 −12

Conversation

bglgwyng
Copy link

@bglgwyng bglgwyng commented Aug 8, 2024 

app :: forall t m. (MonadHeadlessApp t m) => m (Event t ())
app = mdo
  ePostBuild <- getPostBuild

  rec (_, eRequest) <-
        runRequesterT
          ( do
              eX <-
                requesting (ePostBuild $> Const ())
              performEvent_ $ ffor eX (const $ liftIO $ print "Got Response!")
          )
          eResponse'
      (eRequest', eResponse') <-
        matchResponseMapWithRequests
          (\x -> (mkSome x, \case Some x -> unsafeCoerce x))
          eRequest
          eResponse
      eResponse <- pure $ ffor eRequest' $ fmap (\(Some req) -> Some (Identity 1))
  pure never

This minimal example demonstrates a request/response interaction. When a request is made, the corresponding response is generated immediately, as seen in the eResponse definition. However, the current implementation of matchResponseMapWithRequests doesn't correctly handle prompt responses, resulting in "Got Response!" not being printed from eX. This fix ensures that prompt responses are handled correctly, allowing "Got Response!" to be printed as expected.

This was referenced Sep 13, 2024
Open
Open
ryantrinkle
ryantrinkle requested changes Oct 24, 2024
View reviewed changes
Copy link
Member

@ryantrinkle ryantrinkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good patch, but I am slightly concerned about this being prompt. What if someone does something like this?

b <- requesting a
c <- requesting b

Will that create a cycle? It seems like it would. It would be great to add a test for this

Even if that does in fact create a cycle, we could still have something prompt here as an option

src/Reflex/Requester/Base/Internal.hs Outdated Show resolved Hide resolved
@bglgwyng
Copy link
Author

I tried to add test for new matchResponseMapWithRequests, but after I read https://github.com/reflex-frp/reflex/blob/develop/test/RequesterT.hs#L80, I got confused about why the current matchResponseMapWithRequests has passed this test. testMatchRequestsWithResponses responses promptly also, why does the current test pass the current matchResponseMapWithRequests?

@bglgwyng
Copy link
Author

I think we can refactor this code with ??? :: Event a -> Event b -> Event (a, Just b). Do we have something similar?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryantrinkle ryantrinkle ryantrinkle requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bglgwyng @ryantrinkle @ali-abrar